Skip to content

Conversation

@kiendang
Copy link
Contributor

@kiendang kiendang commented May 7, 2025

No description provided.

@kiendang kiendang force-pushed the unzip-dir-executable branch from 05fd1f1 to f82fafc Compare May 7, 2025 11:15
@lihaoyi
Copy link
Member

lihaoyi commented May 7, 2025

Thanks @kiendang ! Seems like we may need to skip this on Windows which doesn't support POSIX permissions

@kiendang
Copy link
Contributor Author

kiendang commented May 7, 2025

I think we should add OWNER_READ as well?

@lihaoyi
Copy link
Member

lihaoyi commented May 7, 2025

@kiendang as long as we can unzip the problematic classes.jar we were hitting in Mill, any follow up permissions issues can be fixed at the callsite. Can you manually try unzipping that jar with your PR just to sanity check that this PR fixes the problem>

@kiendang
Copy link
Contributor Author

kiendang commented May 7, 2025

Hmm then I think the correct fix should be unzipping everything successfully, with the exact permissions specified. Then users fix the permissions at the call site?

For the android case it would be

val unzipped = os.unzip(...)
makeSureDirsHaveExecuteAndReadPerms(unzipped)

Because we don't have control over the jars. If later some jar has -w--w--w- for one of the directory it would crash too.

Would be a simple fix as well. Will submit a separate PR.

@lihaoyi lihaoyi merged commit e051c2c into com-lihaoyi:main May 7, 2025
7 of 8 checks passed
@kiendang kiendang deleted the unzip-dir-executable branch May 7, 2025 15:18
@lefou lefou added this to the 0.11.5 milestone May 8, 2025
lihaoyi pushed a commit that referenced this pull request May 9, 2025
to prevent crash in cases where directories is missing READ/EXECUTE
permission

Added a test that
- creates a zip file with directories without READ/EXECUTE permissions
- `os.unzip` everything successfully
- re-adds READ and EXECUTE permissions to directories

see my and @lefou's comments
#387 (comment)
com-lihaoyi/mill#5048 (comment)

Tested with the android test. Here is the permission fix at the call
site (`AndroidApModule.scala`)

com-lihaoyi/mill@main...kiendang:mill:fix-android-classesjar-unzip#diff-49ebd9c68d1348785194e15a80d62f0845c26386e722d3f5d43429238bbf7616

@lihaoyi @vaslabs
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants